Skip to content

nc-review: severity calibration, issue resolution, and warranted checks - #1209

Merged
will-lamerton merged 2 commits into
mainfrom
fix/nc-review-severity-calibration
Sep 6, 2026
Merged

nc-review: severity calibration, issue resolution, and warranted checks#1209
will-lamerton merged 2 commits into
mainfrom
fix/nc-review-severity-calibration

Conversation

@will-lamerton

@will-lamerton will-lamerton commented Sep 6, 2026

Copy link
Copy Markdown
Member

Run 5 produced a genuinely good review — accurate, specific, verified against the diff — and then mislabelled it. Five real findings, every one marked advisory, verdict clean. So a PR with five things to fix got agent:clean, which is the label a maintainer skims. Copilot read the same PR as needing amends, and Copilot was right.

Two causes, both in the rubric.

1. Confidence and severity were conflated

The old text put "anything you are less than confident about" in the advisory bucket. Those are orthogonal axes — a finding can be high-impact and uncertain, and that wording forced everything uncertain into the low bucket regardless of how much it mattered.

Severity is now rated by impact alone. Uncertainty goes in the detail text ("I could not verify whether X handles Y"), and there is a separate instruction: if you cannot assert a finding at all, do not file it. A hedged real finding beats a silent one; a confident wrong one is worse than both.

2. Binary severity forced the bad verdict

With only blocking / advisory, findings that individually do not block a merge collapse into "clean". Added a middle level:

Meaning
🔴 blocking Do not merge until resolved. Correctness bugs, security, broken contracts, duplicates, a new feature with no test.
🟠 important A human reviewer would ask for a change before approving.
nit Genuinely optional.

The worked example for important is the case run 5 actually found: a test that asserts a failure path which silently succeeds in some environments, so it passes while proving nothing. A green suite makes that more dangerous, not less — which is precisely why it belongs above nit.

There is also a calibration note: if you find yourself marking everything a nit, you are under-calling. Would a careful human reviewer ask for a change before approving? "The maintainer could merge anyway" is true of almost everything and is not the test.

The verdict is now derived, not trusted

The workflow computes it from the findings instead of taking the model's word:

  • any blockingneeds-work
  • any findings at all → comments
  • none → clean

A mismatch between the model's claim and the derived value is logged as a workflow notice, so drift is visible rather than silent.

Three labels, mutually exclusive; the other two are removed unconditionally so a /re-review cannot leave a stale pair. The heading now carries the tally — comments — 2 important, 3 nits — so it cannot disagree with the table underneath it.

Verified

Replaying run 5's actual findings through the new logic:

model claimed: clean
derived:       comments — 2 important, 3 nits
label:         agent:comments  (removes agent:clean, agent:needs-work)

Honest caveat

The verdict derivation is deterministic and will hold. The important-vs-nit split still depends on the model applying the new guidance, and I assigned those severities by hand in the replay above. Whether it actually calls the root-permissions test important rather than nit is the thing to watch on the next real run.


Second commit: issue resolution and whether the PR was warranted

Added to this PR rather than a separate one — it touches the same two files and would have conflicted.

Two gaps

The rubric checked scope creep — did the PR do more than the issue asked — but never completeness: did it do all of it. And it never asked whether the change should exist at all.

Neither was answerable, because the agent was never given the issue. Linked issues are now fetched with full body, title, state and labels, and included in the context. Only GraphQL exposes closingIssuesReferences; gh pr view --json does not have the field in gh 2.69.

Issue resolution

A partial fix on a PR that says "Closes #N" is important at minimum. Merging it closes the issue, and whatever is left over is silently lost — nobody returns to a closed issue.

It is asked to name which parts of a multi-part issue remain, and to say so when the issue is vague enough that several readings are defensible rather than inventing a standard the author could not have known.

Warranted

Maintainer attention is the scarcest resource in this project, and an unnecessary PR costs it whether or not the code is correct. The check asks: is there a real problem, does the codebase already do this, is it churn against working code, does it fight the direction set in CLAUDE.md.

This one needed guardrails and has them. It is the most discouraging thing the bot can say to a contributor, so the rubric states explicitly:

  • small PRs from first-time contributors are how people start
  • "this is small" is not a reason to reject anything
  • an undiscussed but justified change is a conversation, not a rejection
  • do not file this finding at all unless the case is clear

Coverage

43 of 66 open nanocoder PRs link an issue, so the resolution check has real coverage — and the 23 without are exactly the population the warranted check exists for.

Run 5 produced a good review and then mislabelled it. Five real findings, all
marked advisory, verdict "clean" — so a PR with five things to fix got
agent:clean, which is the label a maintainer skims. Copilot read the same PR as
needing amends and was right.

Two causes, both in the rubric I wrote.

1. Confidence and severity were conflated. The old text put "anything you are
   less than confident about" in the advisory bucket. Those are orthogonal: a
   finding can be high-impact and uncertain. Severity is now rated by impact
   alone, with uncertainty expressed in the detail text instead — and an
   explicit instruction not to file anything it cannot assert at all.

2. Binary severity forced the bad verdict. Findings that individually do not
   block a merge collapsed into "clean". There is now a middle level,
   `important`: a human reviewer would ask for a change before approving. The
   test-that-passes-without-exercising-anything case is given as the worked
   example, since a green suite makes that more dangerous rather than less.

The verdict is now derived from the findings in the workflow rather than taken
from the model: any blocking -> needs-work, any findings -> comments, none ->
clean. A mismatch is logged as a notice. Three labels, mutually exclusive, with
the other two removed unconditionally so a re-review cannot leave a stale pair.

The heading now carries the tally ("comments — 2 important, 3 nits") so it
cannot disagree with the table beneath it.

Replaying run 5's findings through the new logic yields
"comments — 2 important, 3 nits" and agent:comments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this - or run pnpm changeset --empty to record that intentionally.

@github-actions github-actions Bot added the area:ci GitHub Actions and CI label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

nc-review: no blocking issues

The PR splits nc-review severity from confidence (three levels instead of two, with uncertainty now expressed in the detail text) and derives the verdict from the findings list rather than trusting the model's claim. The workflow changes correctly implement both: the bash now counts findings by severity, derives verdict from those counts, logs a notice on claimed-vs-derived mismatch, removes the two non-matching labels unconditionally, builds a heading tally, and renders the right emoji per severity. Scope is contained to the review tooling only and there are no duplicates among the open PRs.


Automated code review — correctness, security, design, tests, plus duplicates and scope. Advisory: a human still decides. Not a substitute for review, and not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:clean nc-review had nothing to raise label Sep 6, 2026
Two gaps. The rubric checked scope creep — did the PR do more than the issue
asked — but never completeness, whether it did all of it. And it never asked
whether the change should exist at all.

Neither was answerable before, because the agent was never given the issue.
Linked issues are now fetched with their full bodies, titles, state and labels
and included in the context. Only GraphQL exposes closingIssuesReferences;
`gh pr view --json` does not have the field in gh 2.69.

Issue resolution. A partial fix on a PR that says "Closes #N" is `important` at
minimum: merging it closes the issue and the remainder is silently lost, because
nobody returns to a closed issue. The rubric asks the agent to name which parts
of a multi-part issue remain, and to say so when the issue is vague enough that
several readings are defensible rather than inventing a standard the author
could not have known.

Warranted. Maintainer attention is the scarcest resource here, and an
unnecessary PR costs it whether or not the code is correct. Judges whether there
is a real problem, whether the codebase already does this, whether it is churn
against working code, and whether it fights the direction set in CLAUDE.md.

That last one needed guardrails, so it has them: small PRs from first-time
contributors are how people start, "this is small" is not a reason to reject
anything, an undiscussed but justified change is a conversation rather than a
rejection, and the agent is told not to file the finding at all unless the case
is clear. It is the most discouraging thing it can say to a contributor.

43 of 66 open nanocoder PRs link an issue, so both checks have real coverage and
the 23 without are exactly the population the warranted check exists for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT
@will-lamerton will-lamerton changed the title fix(nc-review): three severity levels, verdict derived not trusted nc-review: severity calibration, issue resolution, and warranted checks Sep 6, 2026
@will-lamerton
will-lamerton merged commit 0f16eac into main Sep 6, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:clean nc-review had nothing to raise area:ci GitHub Actions and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant